Skip to content

pallet-xcm: add new flexible transfer_assets() call/extrinsic#2388

Merged
acatangiu merged 32 commits intoparitytech:masterfrom
acatangiu:xcm-emulator-foreign-assets-transfer-tests
Dec 6, 2023
Merged

pallet-xcm: add new flexible transfer_assets() call/extrinsic#2388
acatangiu merged 32 commits intoparitytech:masterfrom
acatangiu:xcm-emulator-foreign-assets-transfer-tests

Conversation

@acatangiu
Copy link
Copy Markdown
Contributor

@acatangiu acatangiu commented Nov 17, 2023

Motivation (+testing)

Enable easy ForeignAssets transfers using pallet-xcm

We had just previously added capabilities to teleport fees during reserve-based transfers, but what about reserve-transferring fees when needing to teleport some non-fee asset?

This PR aligns everything under either explicit reserve-transfer, explicit teleport, or this new flexible transfer_assets() which can mix and match as needed with fewer artificial constraints imposed to the user.

This will enable, for example, a (non-system) parachain to teleport their ForeignAssets assets to AssetHub while using DOT to pay fees.
(the assets are teleported - as foreign assets should from their owner chain - while DOT used for fees can only be reserve-based transferred between said parachain and AssetHub).

Added xcm-emulator tests for this scenario ^.

Description

Reverts (limited_)reserve_transfer_assets to only allow reserve-based transfers for all assets including fees.

Similarly (limited_)teleport_assets only allows teleports for all assets including fees.

For complex combinations of asset transfers where assets and fees may have different reserves or different reserve/teleport trust configurations, users can use the newly added transfer_assets() extrinsic which is more flexible in allowing more complex scenarios.

assets (excluding fees) must have same reserve location or otherwise be teleportable to dest.
No limitations imposed on fees.

  • for local reserve: transfer assets to sovereign account of destination chain and forward a notification XCM to dest to mint and deposit reserve-based assets to beneficiary.
  • for destination reserve: burn local assets and forward a notification to dest chain to withdraw the reserve assets from this chain's sovereign account and deposit them to beneficiary.
  • for remote reserve: burn local assets, forward XCM to reserve chain to move reserves from this chain's SA to dest chain's SA, and forward another XCM to dest to mint and deposit reserve-based assets to beneficiary.
  • for teleports: burn local assets and forward XCM to dest chain to mint/teleport assets and deposit them to beneficiary.

Review notes

Only around 500 lines are prod code (see pallet_xcm/src/lib.rs), the rest of the PR is new tests and improving existing tests.

@acatangiu acatangiu added T6-XCM This PR/Issue is related to XCM. T2-pallets This PR/Issue is related to a particular pallet. labels Nov 17, 2023
@acatangiu acatangiu self-assigned this Nov 17, 2023
@acatangiu acatangiu requested a review from a team as a code owner November 17, 2023 17:26
@acatangiu
Copy link
Copy Markdown
Contributor Author

acatangiu commented Nov 21, 2023

@KiChjang @franciscoaguirre PTAL and let me know if there's arguments against this - if we are aligned, I will then continue with adding the missing pieces to the PR.

@franciscoaguirre
Copy link
Copy Markdown
Contributor

I like not requiring the user to think about teleports or reserve backed transfers, while still keeping the other extrinsics for more specific use-cases and for devs that have to think about this stuff

@acatangiu acatangiu force-pushed the xcm-emulator-foreign-assets-transfer-tests branch from 499f5ce to 4b8fad6 Compare November 28, 2023 11:58
@acatangiu acatangiu requested review from a team November 28, 2023 11:58
@acatangiu acatangiu marked this pull request as draft November 28, 2023 12:23
@paritytech paritytech deleted a comment from paritytech-cicd-pr Nov 29, 2023
Reverts `(limited_)reserve_transfer_assets` to only allow reserve-based transfers
for all `assets` including fees.
Similarly `(limited_)teleport_assets` only allows teleports for all `assets`
including feed.

For complex combinations of asset transfers where assets and fees may have different
reserves or different reserve/teleport trust configurations, users can use the newly
added `transfer_assets()` extrinsic which is more flexible in allowing more complex
scenarios.

`assets` (excluding `fees`) must have same reserve location or otherwise be teleportable
to `dest`.
No limitations imposed on `fees`.

- for local reserve: transfer assets to sovereign account of destination chain and
  forward a notification XCM to `dest` to mint and deposit reserve-based assets to
  `beneficiary`.
- for destination reserve: burn local assets and forward a notification to `dest` chain
  to withdraw the reserve assets from this chain's sovereign account and deposit them
  to `beneficiary`.
- for remote reserve: burn local assets, forward XCM to reserve chain to move reserves
  from this chain's SA to `dest` chain's SA, and forward another XCM to `dest` to mint
  and deposit reserve-based assets to `beneficiary`.
- for teleports: burn local assets and forward XCM to `dest` chain to mint/teleport
  assets and deposit them to `beneficiary`.
…enpal assets and Asset Hub as foreign assets
@acatangiu acatangiu marked this pull request as ready for review November 30, 2023 06:12
@acatangiu acatangiu force-pushed the xcm-emulator-foreign-assets-transfer-tests branch from 4e2a22c to fbc22b1 Compare November 30, 2023 06:12
@paritytech paritytech deleted a comment from paritytech-cicd-pr Nov 30, 2023
@acatangiu acatangiu added the R1-breaking_change This PR introduces a breaking change and should be highlighted in the upcoming release. label Dec 6, 2023
ordian added a commit that referenced this pull request Dec 11, 2023
* tsv-disabling: (155 commits)
  Fix failing rc-automation GHA (#2648)
  [ci] Return CI_IMAGE variable (#2647)
  Support querying peer reputation (#2392)
  [ci] Update rust to 1.74 (#2545)
  Relax approval requirements on CI files (#2564)
  Added AllSiblingSystemParachains matcher to be used at a parachain level (#2422)
  Improve polkadot sdk docs (#2631)
  Bridges subtree update (#2602)
  pallet-xcm: add new flexible `transfer_assets()` call/extrinsic (#2388)
  [ci] Move rust-features.sh script to .gitlab folder (#2630)
  Bump parity-db from 0.4.10 to 0.4.12 (#2635)
  sp-core: Rename VrfOutput to VrfPreOutput (#2534)
  chore: fix typo (#2596)
  Bump tracing-core from 0.1.31 to 0.1.32 (#2618)
  chore: fixed std wasm build of xcm (#2535)
  Fix PRdoc that have been previously drafted with older schema (#2623)
  Github Workflow migrations (#1574)
  Bridges update subtree (#2625)
  PVF: Add Secure Validator Mode (#2486)
  statement-distribution: Add tests for incoming acknowledgements (#2498)
  ...
ordian added a commit that referenced this pull request Dec 15, 2023
* tsv-disabling: (155 commits)
  Fix failing rc-automation GHA (#2648)
  [ci] Return CI_IMAGE variable (#2647)
  Support querying peer reputation (#2392)
  [ci] Update rust to 1.74 (#2545)
  Relax approval requirements on CI files (#2564)
  Added AllSiblingSystemParachains matcher to be used at a parachain level (#2422)
  Improve polkadot sdk docs (#2631)
  Bridges subtree update (#2602)
  pallet-xcm: add new flexible `transfer_assets()` call/extrinsic (#2388)
  [ci] Move rust-features.sh script to .gitlab folder (#2630)
  Bump parity-db from 0.4.10 to 0.4.12 (#2635)
  sp-core: Rename VrfOutput to VrfPreOutput (#2534)
  chore: fix typo (#2596)
  Bump tracing-core from 0.1.31 to 0.1.32 (#2618)
  chore: fixed std wasm build of xcm (#2535)
  Fix PRdoc that have been previously drafted with older schema (#2623)
  Github Workflow migrations (#1574)
  Bridges update subtree (#2625)
  PVF: Add Secure Validator Mode (#2486)
  statement-distribution: Add tests for incoming acknowledgements (#2498)
  ...
@Polkadot-Forum
Copy link
Copy Markdown

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-5-0/5358/1

bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 10, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 10, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 10, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 11, 2024
Agusrodri pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Jan 11, 2024
…tytech#2388)

We had just previously added capabilities to teleport fees during
reserve-based transfers, but what about reserve-transferring fees when
needing to teleport some non-fee asset?

This PR aligns everything under either explicit reserve-transfer,
explicit teleport, or this new flexible `transfer_assets()` which can
mix and match as needed with fewer artificial constraints imposed to the
user.

This will enable, for example, a (non-system) parachain to teleport
their `ForeignAssets` assets to AssetHub while using DOT to pay fees.
(the assets are teleported - as foreign assets should from their owner
chain - while DOT used for fees can only be reserve-based transferred
between said parachain and AssetHub).

Added `xcm-emulator` tests for this scenario ^.

Reverts `(limited_)reserve_transfer_assets` to only allow reserve-based
transfers for all `assets` including fees.

Similarly `(limited_)teleport_assets` only allows teleports for all
`assets` including fees.

For complex combinations of asset transfers where assets and fees may
have different reserves or different reserve/teleport trust
configurations, users can use the newly added `transfer_assets()`
extrinsic which is more flexible in allowing more complex scenarios.

`assets` (excluding `fees`) must have same reserve location or otherwise
be teleportable to `dest`.
No limitations imposed on `fees`.

- for local reserve: transfer assets to sovereign account of destination
chain and forward a notification XCM to `dest` to mint and deposit
reserve-based assets to `beneficiary`.
- for destination reserve: burn local assets and forward a notification
to `dest` chain to withdraw the reserve assets from this chain's
sovereign account and deposit them to `beneficiary`.
- for remote reserve: burn local assets, forward XCM to reserve chain to
move reserves from this chain's SA to `dest` chain's SA, and forward
another XCM to `dest` to mint and deposit reserve-based assets to
`beneficiary`.
- for teleports: burn local assets and forward XCM to `dest` chain to
mint/teleport assets and deposit them to `beneficiary`.

Only around 500 lines are prod code (see `pallet_xcm/src/lib.rs`), the
rest of the PR is new tests and improving existing tests.

---------

Co-authored-by: command-bot <>
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 12, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 12, 2024
@Polkadot-Forum
Copy link
Copy Markdown

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/8

@brenzi
Copy link
Copy Markdown
Contributor

brenzi commented Feb 14, 2024

This works for our parachain 👍
However: can it be that teleporting our native token to asset hub only works if the order of assets in the transferAsset call is (TEER, ROC, feeAsset = 1)?
I first tried (ROC, TEER, feeAsset=0) but that didn't work

@joepetrowski
Copy link
Copy Markdown
Contributor

I will leave it to @acatangiu to answer, but that does seem strange to me.

@acatangiu
Copy link
Copy Markdown
Contributor Author

acatangiu commented Feb 15, 2024

This works for our parachain 👍 However: can it be that teleporting our native token to asset hub only works if the order of assets in the transferAsset call is (TEER, ROC, feeAsset = 1)? I first tried (ROC, TEER, feeAsset=0) but that didn't work

Like the other extrinsics, transferAsset() takes the assets to transfer through a Box<VersionedMultiAssets> parameter.

MultiAssets object internally sorts the list of assets (I'm not sure where this requirement to be sorted comes from, maybe required for encoding/decoding?).

So my guess is you're doing something like:

let assets: MultiAssets = vec![TEER, ROC].into();    // <-- this works
let fee_idx = 1;                                     // ok

let assets: MultiAssets = vec![ROC, TEER].into();    // <-- still works, but `fee_idx` should still be `1` because of sorting :(
let fee_idx = 0; // <-- reading this looks like it's pointing to ROC, but because of the sorting done by `.into()`, TEER is actually idx 0 and ROC is idx 1

(local assets will always go first because of parents=0 vs ROC which has parents=1)

and I know... this is annoying and bad API design - we could change extrinsic to take actual fee asset instead of index, but would be a breaking change..

Also here is an example helper function I used in tests to get the right index for transfers.

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…tytech#2388)

# Motivation (+testing)

### Enable easy `ForeignAssets` transfers using `pallet-xcm` 

We had just previously added capabilities to teleport fees during
reserve-based transfers, but what about reserve-transferring fees when
needing to teleport some non-fee asset?

This PR aligns everything under either explicit reserve-transfer,
explicit teleport, or this new flexible `transfer_assets()` which can
mix and match as needed with fewer artificial constraints imposed to the
user.

This will enable, for example, a (non-system) parachain to teleport
their `ForeignAssets` assets to AssetHub while using DOT to pay fees.
(the assets are teleported - as foreign assets should from their owner
chain - while DOT used for fees can only be reserve-based transferred
between said parachain and AssetHub).

Added `xcm-emulator` tests for this scenario ^.

# Description

Reverts `(limited_)reserve_transfer_assets` to only allow reserve-based
transfers for all `assets` including fees.

Similarly `(limited_)teleport_assets` only allows teleports for all
`assets` including fees.
    
For complex combinations of asset transfers where assets and fees may
have different reserves or different reserve/teleport trust
configurations, users can use the newly added `transfer_assets()`
extrinsic which is more flexible in allowing more complex scenarios.

`assets` (excluding `fees`) must have same reserve location or otherwise
be teleportable to `dest`.
No limitations imposed on `fees`.

- for local reserve: transfer assets to sovereign account of destination
chain and forward a notification XCM to `dest` to mint and deposit
reserve-based assets to `beneficiary`.
- for destination reserve: burn local assets and forward a notification
to `dest` chain to withdraw the reserve assets from this chain's
sovereign account and deposit them to `beneficiary`.
- for remote reserve: burn local assets, forward XCM to reserve chain to
move reserves from this chain's SA to `dest` chain's SA, and forward
another XCM to `dest` to mint and deposit reserve-based assets to
`beneficiary`.
- for teleports: burn local assets and forward XCM to `dest` chain to
mint/teleport assets and deposit them to `beneficiary`.

## Review notes

Only around 500 lines are prod code (see `pallet_xcm/src/lib.rs`), the
rest of the PR is new tests and improving existing tests.

---------

Co-authored-by: command-bot <>
@Polkadot-Forum
Copy link
Copy Markdown

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-transfer-reserve-non-reserve-assets-in-a-single-xcm/2808/8

@Polkadot-Forum
Copy link
Copy Markdown

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-user-and-developer-experience-improvements/4511/21

@acatangiu acatangiu deleted the xcm-emulator-foreign-assets-transfer-tests branch June 25, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R1-breaking_change This PR introduces a breaking change and should be highlighted in the upcoming release. T2-pallets This PR/Issue is related to a particular pallet. T6-XCM This PR/Issue is related to XCM.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants